Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't check twice if there is a ':' in content (EmojiFilter) #141

Merged
merged 1 commit into from
Sep 15, 2014

Conversation

Razer6
Copy link
Contributor

@Razer6 Razer6 commented Sep 7, 2014

Second check if there is a : in a text is not needed, since it's already done before.

@simeonwillbanks
Copy link
Contributor

👍

@@ -17,7 +17,7 @@ class EmojiFilter < Filter
def call
doc.search('text()').each do |node|
content = node.to_html
next if !content.include?(':')
next unless content.include?(':')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to back this change out, but for future reference, keep PR's focused to what you're trying to solve to make code reviews easier 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this change, because it was directly related to the refactored LOC. I should have promoted this change more clearly in the PR description. Thank's fore merging anyway 🍻

jch added a commit that referenced this pull request Sep 15, 2014
Don't check twice if there is a ':' in content (EmojiFilter)
@jch jch merged commit f869a4b into gjtorikian:master Sep 15, 2014
@jch
Copy link
Contributor

jch commented Sep 15, 2014

Nice cleanup!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants